Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate AuditorAware to an Optional<> for spring boot 1 to 2 migration #614

Merged
merged 18 commits into from
Nov 1, 2024

Conversation

Jenson3210
Copy link
Contributor

@Jenson3210 Jenson3210 commented Oct 30, 2024

@Jenson3210
Copy link
Contributor Author

@timtebeek I've added 4 //TODO Question for TIM:'s.
If you could have a look and help me out in understanding the issue? 🙏

Thanks!

@Jenson3210
Copy link
Contributor Author

@timtebeek Can we also go over the changes of the bot once?
I am seeing stuff I would not perse just commit as suggestion.

For example: where did the maybeAddImport go?
Screenshot 2024-10-30 at 22 09 42

@timtebeek
Copy link
Contributor

@timtebeek Can we also go over the changes of the bot once? I am seeing stuff I would not perse just commit as suggestion.

For example: where did the maybeAddImport go?

Hi yes unfortunately the .patch file is correct, but once converted into comments by https://github.com/googleapis/code-suggester it's sometimes a little off when crossing multiple lines. I have yet to fix that external tool, or write my own.

You can also run the recipes directly if you prefer, possibly through the Moderne CLI to make it most convenient:
https://docs.openrewrite.org/recipes/recipes/openrewritebestpractices

timtebeek and others added 2 commits October 30, 2024 23:23
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@Jenson3210
Copy link
Contributor Author

Okay. Makes sense.

On my private pc I do not have moderne cli set up yet as I do not have the access token etc.

Would that be possible for the open source projects (like open rewrite itself)?

timtebeek and others added 2 commits October 30, 2024 23:32
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@timtebeek
Copy link
Contributor

Okay. Makes sense.

On my private pc I do not have moderne cli set up yet as I do not have the access token etc.

Would that be possible for the open source projects (like open rewrite itself)?

Yes for OSS you would not even need a Moderne license; Just a Moderne public instance personal access token to download recipes and LSTs.

Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see how much of this you've covered already; I've applied some fixes already and left a few comments for other work left to do. Didn't quite finish the last failing test, but we can revisit that once the other items are crossed off.

@Jenson3210
Copy link
Contributor Author

Hi @timtebeek ,

I've pushed some comments you made in the past 3 rounds of rewiew.
There are still 2 tests which I cannot get fixed with the Missing information:

  • org.openrewrite.java.spring.data.MigrateAuditorAwareToOptionalTest#dontRewriteLambdaBlock
  • org.openrewrite.java.spring.data.MigrateAuditorAwareToOptionalTest#dontRewriteLambdaLiteral

Strangly, both are in the don't -> lambda part. So it looks like the lambda in the test code can't be parsed.
The other negated entries are fine :/

Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see! The last remaining failing tests were using a method signature from 2.x as opposed to 1.x, so changing the parser classpath for those tests while still asserting no change is made helped get those to pass.

There might be some scenarios not covered like a class defined in a separate file outside of an immediate return, but we can tackle those separately if they even occur in your wider scan there.

Thanks again!

@timtebeek timtebeek added recipe Recipe requested boot-2.0 labels Nov 1, 2024
@timtebeek timtebeek merged commit b6b1532 into openrewrite:main Nov 1, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
boot-2.0 recipe Recipe requested
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

spring-data-commons changed AuditorAware from 1.x to 2.x
2 participants